Skip to content

BUGFIX: Remove broken @Flow\IgnoreValidation from ContentRepository context#3411

Merged
mficzel merged 1 commit intoneos:masterfrom
mficzel:bugfix/removeBrokenValidationAnnotation
Aug 17, 2021
Merged

BUGFIX: Remove broken @Flow\IgnoreValidation from ContentRepository context#3411
mficzel merged 1 commit intoneos:masterfrom
mficzel:bugfix/removeBrokenValidationAnnotation

Conversation

@mficzel
Copy link
Copy Markdown
Member

@mficzel mficzel commented Aug 16, 2021

This fixes the following error on master:

Argument 1 passed to Neos\Flow\Annotations\IgnoreValidation::__construct() must be of the type string, null given, called in /var/www/html/Packages/Libraries/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php on line 971
Type: TypeError
  File:
Packages/Framework/Neos.Flow/Classes/Annotations/IgnoreValidation.php
  Line: 41

  Type: Neos\Flow\Core\Booting\Exception\SubProcessException
  Code: 1355480641
  File: Packages/Framework/Neos.Flow/Classes/Core/Booting/Scripts.php
  Line: 712

Since the variable with the annotation is populated in the constructor with a fresh instance the need of this annotation is at least questionable.

kdambekalns
kdambekalns approved these changes Aug 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, why was this ever there?

@kdambekalns
Copy link
Copy Markdown
Member

kdambekalns commented Aug 16, 2021

Interesting test failures, though. Why is it installing 7.1.x of the Flow dev collection?

@kdambekalns kdambekalns requested review from albe and kitsunet August 16, 2021 15:52
@kdambekalns
Copy link
Copy Markdown
Member

kdambekalns commented Aug 16, 2021

Regarding the annotation itself: It was added with e09d551 – whether by myself or through a merge hiccup I cannot really tell. But it looks completely disconnected from the actual intention of that change, so I assume it was an oversight.

@kdambekalns
Copy link
Copy Markdown
Member

kdambekalns commented Aug 16, 2021

Interesting test failures, though. Why is it installing 7.1.x of the Flow dev collection?

Probably fixed with #3412

@mficzel
Copy link
Copy Markdown
Member Author

mficzel commented Aug 16, 2021

Yes once ##3412 is applied the error here occurs in the tests.

@mficzel mficzel merged commit d1a0f32 into neos:master Aug 17, 2021
@bwaidelich
Copy link
Copy Markdown
Member

bwaidelich commented Nov 19, 2021

I'm afraid this introduced a regression:

ValidatorResolver::buildBaseValidatorConjunction() explicitly checks for this annotation to skip properties from being validated (see https://github.com/neos/flow-development-collection/blob/a3acde9d245a19d6ac32606ac4edb18bd68fb9e8/Neos.Flow/Classes/Validation/ValidatorResolver.php#L309)

@mficzel
Copy link
Copy Markdown
Member Author

mficzel commented Nov 19, 2021

If this has a purpose the we have to revert the change and fix the test problem differently.

@bwaidelich bwaidelich mentioned this pull request Nov 19, 2021
Closed
@albe albe mentioned this pull request Nov 19, 2021
Merged
@albe
Copy link
Copy Markdown
Contributor

albe commented Nov 19, 2021

Should be reverted once neos/flow-development-collection#2632 has landed, since the missing annotation caused the regression in #3515

@bwaidelich bwaidelich mentioned this pull request Nov 22, 2021
Merged
@bwaidelich
Copy link
Copy Markdown
Member

bwaidelich commented Nov 22, 2021

=> #3516

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kdambekalns kdambekalns kdambekalns approved these changes

@kitsunet kitsunet Awaiting requested review from kitsunet

+1 more reviewer

@albe albe albe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mficzel @kdambekalns @bwaidelich @albe